Menu
▾
▴
[Openvpn-devel] [M] Change in openvpn[master]: win32: stop cmd.exe from expanding variables in quoted arguments
|
From: cron2 (C. Review) <ge...@op...> - 2026-09-07 16:04:20
|
cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1895?usp=email ) 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 <le...@op...> Acked-by: Razvan Cojocaru <ra...@ma...> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1895 Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg38985.html Signed-off-by: Gert Doering <ge...@gr...> --- 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(-) 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: merged Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I70dfdc70778458b78f80b8d0b0b786f18fe4fc76 Gerrit-Change-Number: 1895 Gerrit-PatchSet: 2 Gerrit-Owner: stipa <lst...@gm...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: razvanc <ra...@ma...> Gerrit-CC: openvpn-devel <ope...@li...> |
