[Openvpn-devel] tun.c: fix "use after free" error

Message ID 20200312060829.19468-1-lstipakov@gmail.com
State Accepted
Headers show
Series [Openvpn-devel] tun.c: fix "use after free" error | expand

Commit Message

Lev Stipakov March 11, 2020, 7:08 p.m. UTC
From: Lev Stipakov <lev@openvpn.net>

Commit 509c45f has factored out code blocks of open_tun()
into separate functions and introduced "use after free" bug:

Variable "device_guid" is allocated inside tun_open_device()
function and used outside of it. Allocation happens with
local gc_arena, which is freed at the end of tun_open_device(),
making futher access to "device_guid" invalid.

Fix by ensuring that gc_arena scope covers all access to "device_guid".

Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
 src/openvpn/tun.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

Comments

Simon Rozman March 12, 2020, 3:26 p.m. UTC | #1
Nice catch, Lev. The patch indeed fixes an UAC.

Compiled and tested it with MSVC.

Acked-by: Simon Rozman <simon@rozman.si>

Regards, Simon

> -----Original Message-----
> From: Lev Stipakov <lstipakov@gmail.com>
> Sent: Thursday, March 12, 2020 7:08 AM
> To: openvpn-devel@lists.sourceforge.net
> Cc: Lev Stipakov <lev@openvpn.net>
> Subject: [Openvpn-devel] [PATCH] tun.c: fix "use after free" error
> 
> From: Lev Stipakov <lev@openvpn.net>
> 
> Commit 509c45f has factored out code blocks of open_tun() into separate
> functions and introduced "use after free" bug:
> 
> Variable "device_guid" is allocated inside tun_open_device() function
> and used outside of it. Allocation happens with local gc_arena, which is
> freed at the end of tun_open_device(), making futher access to
> "device_guid" invalid.
> 
> Fix by ensuring that gc_arena scope covers all access to "device_guid".
> 
> Signed-off-by: Lev Stipakov <lev@openvpn.net>
> ---
>  src/openvpn/tun.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c index
> 42193d97..c976055e 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -6226,12 +6226,11 @@ tun_try_open_device(struct tuntap *tt, const
> char *device_guid, const struct dev  }
> 
>  static void
> -tun_open_device(struct tuntap *tt, const char *dev_node, const char
> **device_guid)
> +tun_open_device(struct tuntap *tt, const char *dev_node, const char
> +**device_guid, struct gc_arena *gc)
>  {
> -    struct gc_arena gc = gc_new();
> -    const struct tap_reg *tap_reg = get_tap_reg(&gc);
> -    const struct panel_reg *panel_reg = get_panel_reg(&gc);
> -    const struct device_instance_id_interface
> *device_instance_id_interface = get_device_instance_id_interface(&gc);
> +    const struct tap_reg *tap_reg = get_tap_reg(gc);
> +    const struct panel_reg *panel_reg = get_panel_reg(gc);
> +    const struct device_instance_id_interface
> + *device_instance_id_interface = get_device_instance_id_interface(gc);
>      char actual_buffer[256];
> 
>      at_least_one_tap_win(tap_reg);
> @@ -6244,7 +6243,7 @@ tun_open_device(struct tuntap *tt, const char
> *dev_node, const char **device_gui
>          enum windows_driver_type windows_driver =
> WINDOWS_DRIVER_UNSPECIFIED;
> 
>          /* Get the device GUID for the device specified with --dev-
> node. */
> -        *device_guid = get_device_guid(dev_node, actual_buffer,
> sizeof(actual_buffer), &windows_driver, tap_reg, panel_reg, &gc);
> +        *device_guid = get_device_guid(dev_node, actual_buffer,
> + sizeof(actual_buffer), &windows_driver, tap_reg, panel_reg, gc);
> 
>          if (!*device_guid)
>          {
> @@ -6276,7 +6275,7 @@ tun_open_device(struct tuntap *tt, const char
> *dev_node, const char **device_gui
>                                                         tap_reg,
>                                                         panel_reg,
>                                                         &windows_driver,
> -                                                       &gc);
> +                                                       gc);
> 
>              if (!*device_guid)
>              {
> @@ -6304,8 +6303,6 @@ next:
> 
>      msg(M_INFO, "%s device [%s] opened", print_windows_driver(tt-
> >windows_driver), tt->actual_name);
>      tt->adapter_index = get_adapter_index(*device_guid);
> -
> -    gc_free(&gc);
>  }
> 
>  static void
> @@ -6411,13 +6408,16 @@ open_tun(const char *dev, const char *dev_type,
> const char *dev_node, struct tun
>          msg(M_FATAL|M_NOPREFIX, "Unknown virtual device type: '%s'",
> dev);
>      }
> 
> -    tun_open_device(tt, dev_node, &device_guid);
> +    struct gc_arena gc = gc_new(); /* used also for device_guid
> allocation */
> +    tun_open_device(tt, dev_node, &device_guid, &gc);
> 
>      if (tt->windows_driver == WINDOWS_DRIVER_TAP_WINDOWS6)
>      {
>          tuntap_post_open(tt, device_guid);
>      }
> 
> +    gc_free(&gc);
> +
>      /*netcmd_semaphore_release ();*/
>  }
> 
> --
> 2.17.1
> 
> 
> 
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Gert Doering March 13, 2020, 9:47 p.m. UTC | #2
Your patch has been applied to the master branch.

No testing done on my side, but from a stare-at-code perspective this
looks reasonable & necessary.  Thanks.

commit 5d28b47c51f4fcef631f9dae83f9cbc7120c6812
Author: Lev Stipakov
Date:   Thu Mar 12 08:08:29 2020 +0200

     tun.c: fix 'use after free' error

     Signed-off-by: Lev Stipakov <lev@openvpn.net>
     Acked-by: Simon Rozman <simon@rozman.si>
     Message-Id: <20200312060829.19468-1-lstipakov@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19547.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 42193d97..c976055e 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -6226,12 +6226,11 @@  tun_try_open_device(struct tuntap *tt, const char *device_guid, const struct dev
 }
 
 static void
-tun_open_device(struct tuntap *tt, const char *dev_node, const char **device_guid)
+tun_open_device(struct tuntap *tt, const char *dev_node, const char **device_guid, struct gc_arena *gc)
 {
-    struct gc_arena gc = gc_new();
-    const struct tap_reg *tap_reg = get_tap_reg(&gc);
-    const struct panel_reg *panel_reg = get_panel_reg(&gc);
-    const struct device_instance_id_interface *device_instance_id_interface = get_device_instance_id_interface(&gc);
+    const struct tap_reg *tap_reg = get_tap_reg(gc);
+    const struct panel_reg *panel_reg = get_panel_reg(gc);
+    const struct device_instance_id_interface *device_instance_id_interface = get_device_instance_id_interface(gc);
     char actual_buffer[256];
 
     at_least_one_tap_win(tap_reg);
@@ -6244,7 +6243,7 @@  tun_open_device(struct tuntap *tt, const char *dev_node, const char **device_gui
         enum windows_driver_type windows_driver = WINDOWS_DRIVER_UNSPECIFIED;
 
         /* Get the device GUID for the device specified with --dev-node. */
-        *device_guid = get_device_guid(dev_node, actual_buffer, sizeof(actual_buffer), &windows_driver, tap_reg, panel_reg, &gc);
+        *device_guid = get_device_guid(dev_node, actual_buffer, sizeof(actual_buffer), &windows_driver, tap_reg, panel_reg, gc);
 
         if (!*device_guid)
         {
@@ -6276,7 +6275,7 @@  tun_open_device(struct tuntap *tt, const char *dev_node, const char **device_gui
                                                        tap_reg,
                                                        panel_reg,
                                                        &windows_driver,
-                                                       &gc);
+                                                       gc);
 
             if (!*device_guid)
             {
@@ -6304,8 +6303,6 @@  next:
 
     msg(M_INFO, "%s device [%s] opened", print_windows_driver(tt->windows_driver), tt->actual_name);
     tt->adapter_index = get_adapter_index(*device_guid);
-
-    gc_free(&gc);
 }
 
 static void
@@ -6411,13 +6408,16 @@  open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
         msg(M_FATAL|M_NOPREFIX, "Unknown virtual device type: '%s'", dev);
     }
 
-    tun_open_device(tt, dev_node, &device_guid);
+    struct gc_arena gc = gc_new(); /* used also for device_guid allocation */
+    tun_open_device(tt, dev_node, &device_guid, &gc);
 
     if (tt->windows_driver == WINDOWS_DRIVER_TAP_WINDOWS6)
     {
         tuntap_post_open(tt, device_guid);
     }
 
+    gc_free(&gc);
+
     /*netcmd_semaphore_release ();*/
 }