diff mbox series

[Openvpn-devel,v1] win32: stop cmd.exe from expanding variables in quoted arguments

Message ID 20260907144351.22085-1-gert@greenie.muc.de
State New
Headers
Series [Openvpn-devel,v1] win32: stop cmd.exe from expanding variables in quoted arguments |

Commit Message

Gert Doering Sept. 7, 2026, 2:43 p.m. UTC 
  From: Lev Stipakov <lev@openvpn.net>

Commit cf08504 quotes arguments that contain cmd.exe metacharacters, so a
certificate subject passed to --tls-verify cannot start a second command
when CreateProcess() runs a .bat or .cmd hook.

Quoting is not enough for %VAR% and !VAR!: cmd.exe expands those even
inside double quotes, and OpenVPN exports peer-controlled certificate
fields into the child environment. A subject that puts a quote and an
operator in one field (O=BREAK"&whoami&") and references it from another
(CN=%X509_0_O%) expands back into a quote and command operator after
wide_cmd_line() has already replaced the direct quotes, running a command
before the hook can reject the peer.

Replace % and ! in wide_cmd_line() along with the double quotes and CRLF,
so no argument can expand. This reuses two character classes that had no
users, CC_AT and CC_EQUAL, renamed to CC_PERCENT and CC_EXCLAMATION. Add
regression tests for a bare percent or bang, a closed expansion token, and
the reported subject.

This completes the CVE-2026-84256 fix for Windows batch hooks.

GitHub: OpenVPN/openvpn-private-issues#176
Reported-By: Darren Carreras
CVE: 2026-84256
Change-Id: I70dfdc70778458b78f80b8d0b0b786f18fe4fc76
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Razvan Cojocaru <razvanc@mailbox.org>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1895
---

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/+/1895
This mail reflects revision 1 of this Change.

Acked-by according to Gerrit (reflected above):
Razvan Cojocaru <razvanc@mailbox.org>

Comments

Gert Doering Sept. 7, 2026, 4:03 p.m. UTC | #1 
Windows, the platform that never ceases to entertain...

Thanks for taking this on, and thanks to Razvan for reviewing.

I have not tested this, just stared at the code a bit, and did a test
compile (mingw).

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

(I would backport it to 2.6, but since the unit test and win32-util
patch wasn't backported, this does not apply easily - but given that
we do not intend to ever do a windows installer again, this is somewhat
moot.  Upgrade your windows clients to 2.7.x, if you have to deal
with a non-trustworthy or neglicient CA)

commit eb12c84ef08566c4d2d5f3ea1294891339a5b5e9 (master)
commit 4eaacbb9c7215eb30eee2ab4d55162402385c418 (release/2.7)
Author: Lev Stipakov
Date:   Mon Sep 7 16:43:44 2026 +0200

     win32: stop cmd.exe from expanding variables in quoted arguments

     Signed-off-by: Lev Stipakov <lev@openvpn.net>
     Acked-by: Razvan Cojocaru <razvanc@mailbox.org>
     Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1895
     Message-Id: <20260907144351.22085-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg38985.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c
index 5ce2e39..5d00415 100644
--- a/src/openvpn/buffer.c
+++ b/src/openvpn/buffer.c
@@ -943,11 +943,11 @@ 
     {
         return true;
     }
-    if ((flags & CC_AT) && c == '@')
+    if ((flags & CC_PERCENT) && c == '%')
     {
         return true;
     }
-    if ((flags & CC_EQUAL) && c == '=')
+    if ((flags & CC_EXCLAMATION) && c == '!')
     {
         return true;
     }
diff --git a/src/openvpn/buffer.h b/src/openvpn/buffer.h
index 4471697..0a02e94 100644
--- a/src/openvpn/buffer.h
+++ b/src/openvpn/buffer.h
@@ -1660,8 +1660,8 @@ 
 #define CC_SINGLE_QUOTE  (1 << 21) /**< single quote */
 #define CC_DOUBLE_QUOTE  (1 << 22) /**< double quote */
 #define CC_REVERSE_QUOTE (1 << 23) /**< reverse quote */
-#define CC_AT            (1 << 24) /**< at sign */
-#define CC_EQUAL         (1 << 25) /**< equal sign */
+#define CC_PERCENT       (1 << 24) /**< percent sign */
+#define CC_EXCLAMATION   (1 << 25) /**< exclamation mark */
 #define CC_LESS_THAN     (1 << 26) /**< less than sign */
 #define CC_GREATER_THAN  (1 << 27) /**< greater than sign */
 #define CC_PIPE          (1 << 28) /**< pipe */
diff --git a/src/openvpn/win32-util.c b/src/openvpn/win32-util.c
index 4e3819c..e2c8810 100644
--- a/src/openvpn/win32-util.c
+++ b/src/openvpn/win32-util.c
@@ -94,7 +94,10 @@ 
     {
         const char *arg = a->argv[i];
         strcpy(work, arg);
-        string_mod(work, CC_PRINT, CC_DOUBLE_QUOTE | CC_CRLF, '_');
+        /* cmd.exe expands %VAR% and !VAR! even inside quotes, so a value like
+         * %X509_0_O% could turn back into a quote and start a new command.
+         * Replace those along with the double quotes and CRLF. */
+        string_mod(work, CC_PRINT, CC_DOUBLE_QUOTE | CC_CRLF | CC_PERCENT | CC_EXCLAMATION, '_');
         if (i)
         {
             buf_printf(&buf, " ");
diff --git a/tests/unit_tests/openvpn/test_argv.c b/tests/unit_tests/openvpn/test_argv.c
index 5b6e26e..1fc73d8 100644
--- a/tests/unit_tests/openvpn/test_argv.c
+++ b/tests/unit_tests/openvpn/test_argv.c
@@ -271,15 +271,13 @@ 
         { "CN=a,b;c=d", L"script.bat 0 CN=a,b;c=d" },
         /* a space has always forced quoting */
         { "O=Ctrl, CN=y", L"script.bat 0 \"O=Ctrl, CN=y\"" },
-        /* cmd.exe operators */
+        /* cmd.exe operators that quoting neutralizes */
         { "CN=x&ver", L"script.bat 0 \"CN=x&ver\"" },
         { "CN=x|ver", L"script.bat 0 \"CN=x|ver\"" },
         { "CN=x>f", L"script.bat 0 \"CN=x>f\"" },
         { "CN=x<f", L"script.bat 0 \"CN=x<f\"" },
         { "CN=x^f", L"script.bat 0 \"CN=x^f\"" },
-        { "CN=x%f", L"script.bat 0 \"CN=x%f\"" },
         { "CN=x(f)", L"script.bat 0 \"CN=x(f)\"" },
-        { "CN=x!f", L"script.bat 0 \"CN=x!f\"" },
         /* a double quote is replaced, so quoting cannot be broken out of */
         { "CN=a\"b", L"script.bat 0 CN=a_b" },
     };
@@ -300,6 +298,49 @@ 
         gc_free(&gc);
     }
 }
+
+/*
+ * cmd.exe expands %VAR% and !VAR! even inside quotes, and OpenVPN puts
+ * peer-controlled data (e.g. certificate subject fields) on the command line,
+ * so these characters are replaced to stop a value from expanding back into a
+ * quote and command operator.
+ */
+static void
+wide_cmd_line__replaces_cmd_expansion(void **state)
+{
+    static const struct
+    {
+        const char *arg;
+        const WCHAR *expected;
+    } cases[] = {
+        /* a bare percent or bang is replaced */
+        { "CN=x%f", L"script.bat 0 CN=x_f" },
+        { "CN=x!f", L"script.bat 0 CN=x_f" },
+        /* a closed expansion token is replaced, so nothing expands */
+        { "CN=%X509_0_O%", L"script.bat 0 CN=_X509_0_O_" },
+        { "CN=!X509_0_O!", L"script.bat 0 CN=_X509_0_O_" },
+        /* the reported bypass: quotes in one field are already replaced, and
+         * neutralizing % stops %X509_0_O% from re-injecting them */
+        { "O=BREAK\"&whoami&\", CN=%X509_0_O%",
+          L"script.bat 0 \"O=BREAK_&whoami&_, CN=_X509_0_O_\"" },
+    };
+
+    for (size_t i = 0; i < SIZE(cases); i++)
+    {
+        struct gc_arena gc = gc_new();
+        struct argv a = argv_new();
+
+        argv_printf(&a, "%s %d %s", "script.bat", 0, cases[i].arg);
+        assert_int_equal(a.argc, 3);
+
+        WCHAR *cmd_line = wide_cmd_line(&a, &gc);
+        assert_non_null(cmd_line);
+        assert_int_equal(wcscmp(cmd_line, cases[i].expected), 0);
+
+        argv_free(&a);
+        gc_free(&gc);
+    }
+}
 #endif /* _WIN32 */
 
 int
@@ -323,6 +364,7 @@ 
         cmocka_unit_test(argv_insert_head__empty_argv__head_only),
 #ifdef _WIN32
         cmocka_unit_test(wide_cmd_line__quotes_only_what_cmd_would_reinterpret),
+        cmocka_unit_test(wide_cmd_line__replaces_cmd_expansion),
 #endif
     };