diff mbox series

[Openvpn-devel,v3] dns: fix potential NULL pointer dereference

Message ID 20250520073354.17091-1-gert@greenie.muc.de
State Accepted
Headers show
Series [Openvpn-devel,v3] dns: fix potential NULL pointer dereference | expand

Commit Message

Gert Doering May 20, 2025, 7:33 a.m. UTC 
From: Heiko Hund <heiko@ist.eigentlich.net>

Fix issue reported by Coverity (CID 1646952): Dereferencing a pointer
that might be NULL dvf when calling env_set_write_file.

In addition to the fix, inline the write_dns_vars_file() helper function.
Also output a log line in case this error happens, because when it
happens it will hinder communication with the updown runner process, i.e.
setting up / tearing down DNS things will not work as expected.

Change-Id: I275bf939f43577427e14890e7093d63c5213ae5d
Signed-off-by: Heiko Hund <heiko@ist.eigentlich.net>
Acked-by: Gert Doering <gert@greenie.muc.de>
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1026
This mail reflects revision 3 of this Change.

Acked-by according to Gerrit (reflected above):
Gert Doering <gert@greenie.muc.de>

Comments

Gert Doering May 20, 2025, 8:25 a.m. UTC | #1 
Thanks for this.

Found by Coverity - in the case of a problem in platform_create_temp_file(),
the old code would have accessed strlen(NULL) and *boom*.  Unlikely, but
arguably still possible if something changes with --tmpdir while openvpn
is running, or so.

Stared at code v2+v3, like v3 better :-) - tested non-error branch
on Linux and FreeBSD (its own can of worms), works.

Your patch has been applied to the master branch.

commit d1045d19bf4ea1f717edac7c67f20e9d8eb3a261
Author: Heiko Hund
Date:   Tue May 20 09:33:48 2025 +0200

     dns: fix potential NULL pointer dereference

     Signed-off-by: Heiko Hund <heiko@ist.eigentlich.net>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20250520073354.17091-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg31720.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/dns.c b/src/openvpn/dns.c
index 9927961..3c703cc 100644
--- a/src/openvpn/dns.c
+++ b/src/openvpn/dns.c
@@ -688,18 +688,6 @@ 
     return true;
 }
 
-static const char *
-write_dns_vars_file(bool up, const struct options *o, const struct tuntap *tt, struct gc_arena *gc)
-{
-    struct env_set *es = env_set_create(gc);
-    const char *dvf = platform_create_temp_file(o->tmp_dir, "dvf", gc);
-
-    updown_env_set(up, &o->dns_options, tt, es);
-    env_set_write_file(dvf, es);
-
-    return dvf;
-}
-
 static void
 run_up_down_command(bool up, struct options *o, const struct tuntap *tt, struct dns_updown_runner_info *updown_runner)
 {
@@ -709,7 +697,7 @@ 
         return;
     }
 
-    int status;
+    int status = -1;
 
     if (!updown_runner->required)
     {
@@ -728,11 +716,19 @@ 
         }
 
         struct gc_arena gc = gc_new();
-        int rfd = updown_runner->fds[0];
-        int wfd = updown_runner->fds[1];
-        const char *dvf = write_dns_vars_file(up, o, tt, &gc);
-        size_t dvf_size = strlen(dvf) + 1;
+        const char *dvf = platform_create_temp_file(o->tmp_dir, "dvf", &gc);
+        if (!dvf)
+        {
+            msg(M_ERR, "could not create dns vars file");
+            goto out_free;
+        }
 
+        struct env_set *es = env_set_create(&gc);
+        updown_env_set(up, &o->dns_options, tt, es);
+        env_set_write_file(dvf, es);
+
+        int wfd = updown_runner->fds[1];
+        size_t dvf_size = strlen(dvf) + 1;
         while (1)
         {
             ssize_t len = write(wfd, dvf, dvf_size);
@@ -747,6 +743,7 @@ 
             break;
         }
 
+        int rfd = updown_runner->fds[0];
         while (1)
         {
             ssize_t len = read(rfd, &status, sizeof(status));
@@ -761,6 +758,7 @@ 
             break;
         }
 
+out_free:
         gc_free(&gc);
     }