[M] Change in openvpn[master]: win32: stop cmd.exe from expanding variables in quoted arguments

Attention is currently required from: plaisthos.

Hello plaisthos,

I'd like you to do a code review.
Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1895?usp=email

to review the following change.


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

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

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 <[email protected]>
---
M src/openvpn/buffer.c
M src/openvpn/buffer.h
M src/openvpn/win32-util.c
M tests/unit_tests/openvpn/test_argv.c
4 files changed, 53 insertions(+), 8 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/95/1895/1

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
     };
 

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1895?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I70dfdc70778458b78f80b8d0b0b786f18fe4fc76
Gerrit-Change-Number: 1895
Gerrit-PatchSet: 1
Gerrit-Owner: stipa <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>

_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel