diff mbox series

[Openvpn-devel,v2] block-dns using iservice: fix a potential double free

Message ID 20230201170735.2266851-1-selva.nair@gmail.com
State Accepted
Headers show
Series [Openvpn-devel,v2] block-dns using iservice: fix a potential double free | expand

Commit Message

Selva Nair Feb. 1, 2023, 5:07 p.m. UTC 
From: Selva Nair <selva.nair@gmail.com>

- An item added to undo-list was not removed on error, causing
  attempt to free again in Undo().
  Also fix a memory leak possibility in the same context.

Github: fixes OpenVPN/openvpn#232

v2: Split add and delete functions and reuse the delete
function for cleanup.

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---

Same as PR #235 except for DeleteBlockDNS moved up and
the its forward declaration removed.

 src/openvpnserv/interactive.c | 132 ++++++++++++++++++++--------------
 1 file changed, 78 insertions(+), 54 deletions(-)

Comments

Lev Stipakov Feb. 2, 2023, 7:52 a.m. UTC | #1 
Hi,

Stared at the code and tested with/without Citrix DME (which caused
crash) - code is now cleaner
(add/delete separation) and no crash anymore. Next we will fix the driver :)

Acked-by: Lev Stipakov <lstipakov@gmail.com>
Gert Doering Feb. 2, 2023, 8:56 a.m. UTC | #2 
I've stared a bit at the code, and it looks good.  Also, MinGW and GH
compile it just fine ;-) - did not test actual installation.

Your patch has been applied to the master and release/2.6 branch.

commit b761cb9bc942b6077f0b6e2b85a72e33fc618a0f (master)
commit a85257e78f0d9f922941ad981bc4272d0aaf5594 (release/2.6)
Author: Selva Nair
Date:   Wed Feb 1 12:07:35 2023 -0500

     block-dns using iservice: fix a potential double free

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Lev Stipakov <lstipakov@gmail.com>
     Message-Id: <20230201170735.2266851-1-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26130.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 03361d66..a3d43752 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -777,87 +777,111 @@  CmpAny(LPVOID item, LPVOID any)
 }
 
 static DWORD
-HandleBlockDNSMessage(const block_dns_message_t *msg, undo_lists_t *lists)
+DeleteBlockDNS(const block_dns_message_t *msg, undo_lists_t *lists)
 {
     DWORD err = 0;
-    block_dns_data_t *interface_data;
+    block_dns_data_t *interface_data = RemoveListItem(&(*lists)[block_dns], CmpAny, NULL);
+
+    if (interface_data)
+    {
+        err = delete_block_dns_filters(interface_data->engine);
+        if (interface_data->metric_v4 >= 0)
+        {
+            set_interface_metric(msg->iface.index, AF_INET,
+                                 interface_data->metric_v4);
+        }
+        if (interface_data->metric_v6 >= 0)
+        {
+            set_interface_metric(msg->iface.index, AF_INET6,
+                                 interface_data->metric_v6);
+        }
+        free(interface_data);
+    }
+    else
+    {
+        MsgToEventLog(M_ERR, TEXT("No previous block DNS filters to delete"));
+    }
+
+    return err;
+}
+
+static DWORD
+AddBlockDNS(const block_dns_message_t *msg, undo_lists_t *lists)
+{
+    DWORD err = 0;
+    block_dns_data_t *interface_data = NULL;
     HANDLE engine = NULL;
     LPCWSTR exe_path;
 
     exe_path = settings.exe_path;
 
-    if (msg->header.type == msg_add_block_dns)
+    err = add_block_dns_filters(&engine, msg->iface.index, exe_path, BlockDNSErrHandler);
+    if (!err)
     {
-        err = add_block_dns_filters(&engine, msg->iface.index, exe_path, BlockDNSErrHandler);
+        interface_data = malloc(sizeof(block_dns_data_t));
+        if (!interface_data)
+        {
+            err = ERROR_OUTOFMEMORY;
+            goto out;
+        }
+        interface_data->engine = engine;
+        interface_data->index = msg->iface.index;
+        int is_auto = 0;
+        interface_data->metric_v4 = get_interface_metric(msg->iface.index,
+                                                         AF_INET, &is_auto);
+        if (is_auto)
+        {
+            interface_data->metric_v4 = 0;
+        }
+        interface_data->metric_v6 = get_interface_metric(msg->iface.index,
+                                                         AF_INET6, &is_auto);
+        if (is_auto)
+        {
+            interface_data->metric_v6 = 0;
+        }
+
+        err = AddListItem(&(*lists)[block_dns], interface_data);
         if (!err)
         {
-            interface_data = malloc(sizeof(block_dns_data_t));
-            if (!interface_data)
-            {
-                return ERROR_OUTOFMEMORY;
-            }
-            interface_data->engine = engine;
-            interface_data->index = msg->iface.index;
-            int is_auto = 0;
-            interface_data->metric_v4 = get_interface_metric(msg->iface.index,
-                                                             AF_INET, &is_auto);
-            if (is_auto)
-            {
-                interface_data->metric_v4 = 0;
-            }
-            interface_data->metric_v6 = get_interface_metric(msg->iface.index,
-                                                             AF_INET6, &is_auto);
-            if (is_auto)
-            {
-                interface_data->metric_v6 = 0;
-            }
-            err = AddListItem(&(*lists)[block_dns], interface_data);
+            err = set_interface_metric(msg->iface.index, AF_INET,
+                                       BLOCK_DNS_IFACE_METRIC);
             if (!err)
             {
-                err = set_interface_metric(msg->iface.index, AF_INET,
+                err = set_interface_metric(msg->iface.index, AF_INET6,
                                            BLOCK_DNS_IFACE_METRIC);
-                if (!err)
-                {
-                    set_interface_metric(msg->iface.index, AF_INET6,
-                                         BLOCK_DNS_IFACE_METRIC);
-                }
-            }
-        }
-    }
-    else
-    {
-        interface_data = RemoveListItem(&(*lists)[block_dns], CmpAny, NULL);
-        if (interface_data)
-        {
-            engine = interface_data->engine;
-            err = delete_block_dns_filters(engine);
-            engine = NULL;
-            if (interface_data->metric_v4 >= 0)
-            {
-                set_interface_metric(msg->iface.index, AF_INET,
-                                     interface_data->metric_v4);
             }
-            if (interface_data->metric_v6 >= 0)
+            if (err)
             {
-                set_interface_metric(msg->iface.index, AF_INET6,
-                                     interface_data->metric_v6);
+                /* delete the filters, remove undo item and free interface data */
+                DeleteBlockDNS(msg, lists);
+                engine = NULL;
             }
-            free(interface_data);
-        }
-        else
-        {
-            MsgToEventLog(M_ERR, TEXT("No previous block DNS filters to delete"));
         }
     }
 
+out:
     if (err && engine)
     {
         delete_block_dns_filters(engine);
+        free(interface_data);
     }
 
     return err;
 }
 
+static DWORD
+HandleBlockDNSMessage(const block_dns_message_t *msg, undo_lists_t *lists)
+{
+    if (msg->header.type == msg_add_block_dns)
+    {
+        return AddBlockDNS(msg, lists);
+    }
+    else
+    {
+        return DeleteBlockDNS(msg, lists);
+    }
+}
+
 /*
  * Execute a command and return its exit code. If timeout > 0, terminate
  * the process if still running after timeout milliseconds. In that case