diff mbox series

[Openvpn-devel,v1] Validate DNS domain name before powershell invocation

Message ID 20250918173447.32466-1-gert@greenie.muc.de
State Accepted
Headers
Series [Openvpn-devel,v1] Validate DNS domain name before powershell invocation |

Commit Message

Gert Doering Sept. 18, 2025, 5:34 p.m. UTC 
  From: Lev Stipakov <lev@openvpn.net>

Starting from commit

  d383d6e ("win: replace wmic invocation with powershell")

we pass --dhcp-option DOMAIN value to a powershell command
to set DNS domain. Without validation this opens the door
to a command injection atack.

This only allows domain names with chartacters:

  [A-Za-z0-9.-_\x80-\0xff]

Change-Id: I7a57d7b4e84aa2b9c9e71e30520ed468b0e3c278
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Gert Doering <gert@greenie.muc.de>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1198
---

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

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

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

Comments

Gert Doering Sept. 19, 2025, 7:40 a.m. UTC | #1 
So, we received a report that our use of 'wmic.exe' is insafe and could
be exploited by a malicious server pushing a "suitable" DOMAIN - which
could not be reproduced by us or by the reporter.  So this was rejected
as "invalid report" and we did not assign a CVE number.

This said, at about the same time, we integrated a patch that replaces
use of 'wmic.exe' with a powershell command line (because MS is no longer
installing wmic.exe by default, and intents to activey remove it in 25H2)
- and it turns out that the powershell invocation was insecure against
malicious data sent by the server in a PUSH_REPLY.  This problematic code
is NOT in any released OpenVPN version (2.6.14 and 2.7_beta1 have wmic.exe),
so it doesn't get a CVE ID either (CNA rules).

This patch fixes this (before 2.6.15 release ;-) ) by introducing proper
positive-list validation of domain names used in powershell calls.  We
accept UTF8 encoded domain names (= all characters >= 0x80 are allowed),
because it's needed for some environments, and nothing in there translates
into a (power-)shell metacharacter.


Tested on a Win10 system I had around, with binaries built on Ubuntu 22.04
with MinGW.  Running OpenVPN from an elevated cmd prompt does

  "Failed to set DNS domain 'mooh.com'evil' beause it contains invalid
   characters"

Running via the iservice, I get an error in OpenVPN log

  "TUN: adding dns domain failed using service: Die Daten sind
   unzulässig..." (yeah, translations)

and one in Event Log

  "openvpnserv error:
   Failed to set DNS domain 'mooh.com'evil' because it contains invalid
   characters"

which is also how it should be.  With a valid domain, it still works :-)


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

(In master, the domain related code is sufficiently different to require
a separate patch)

commit 6c3afe508b15764eea4e5bdcbaed37c02c281d9a (release/2.6)
Author: Lev Stipakov
Date:   Thu Sep 18 19:34:40 2025 +0200

     Validate DNS domain name before powershell invocation

     Signed-off-by: Lev Stipakov <lev@openvpn.net>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1198
     Message-Id: <20250918173447.32466-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg33071.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/domain_helper.h b/src/openvpn/domain_helper.h
new file mode 100644
index 0000000..f1ecf86
--- /dev/null
+++ b/src/openvpn/domain_helper.h
@@ -0,0 +1,45 @@ 
+/*
+ *  OpenVPN -- An application to securely tunnel IP networks
+ *             over a single UDP port, with support for SSL/TLS-based
+ *             session authentication and key exchange,
+ *             packet encryption, packet authentication, and
+ *             packet compression.
+ *
+ *  Copyright (C) 2025 Lev Stipakov <lev@openvpn.net>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2
+ *  as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+static inline bool
+is_allowed_domain_ascii(unsigned char c)
+{
+    return (c >= 'A' && c <= 'Z')
+           || (c >= 'a' && c <= 'z')
+           || (c >= '0' && c <= '9')
+           || c == '.' || c == '-' || c == '_' || c >= 0x80;
+}
+
+static inline bool
+validate_domain(const char *domain)
+{
+    for (const char *ch = domain; *ch; ++ch)
+    {
+        if (!is_allowed_domain_ascii((unsigned char)*ch))
+        {
+            return false;
+        }
+    }
+
+    return true;
+}
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 99dc490..2784941 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -46,6 +46,7 @@ 
 #include "win32.h"
 #include "block_dns.h"
 #include "networking.h"
+#include "domain_helper.h"
 
 #include "memdbg.h"
 
@@ -390,6 +391,12 @@ 
         return;
     }
 
+    if (add && !validate_domain(tt->options.domain))
+    {
+        msg(M_WARN, "Failed to set DNS domain '%s' because it contains invalid characters", tt->options.domain);
+        return;
+    }
+
     struct argv argv = argv_new();
     argv_printf(&argv,
                 "%s%s -NoProfile -NonInteractive -Command Set-DnsClient -InterfaceIndex %lu -ConnectionSpecificSuffix '%s'",
diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index ca58596..c12d34f 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -40,6 +40,7 @@ 
 #include "validate.h"
 #include "block_dns.h"
 #include "ring_buffer.h"
+#include "domain_helper.h"
 
 #define IO_TIMEOUT  2000 /*ms*/
 
@@ -1216,6 +1217,12 @@ 
 {
     NET_IFINDEX if_index;
 
+    if (!validate_domain(domain))
+    {
+        MsgToEventLog(MSG_FLAGS_ERROR, TEXT("Failed to set DNS domain '%hs' because it contains invalid characters"), domain);
+        return ERROR_INVALID_DATA;
+    }
+
     DWORD err  = ConvertInterfaceNameToIndex(if_name, &if_index);
     if (err != ERROR_SUCCESS)
     {