[M] Change in openvpn[master]: win32: unit-test the CreateProcess() command line quoting

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/+/1892?usp=email

to review the following change.


Change subject: win32: unit-test the CreateProcess() command line quoting
......................................................................

win32: unit-test the CreateProcess() command line quoting

Cover which argument shapes wide_cmd_line() quotes: plain arguments and
the batch delimiters , ; = stay bare, a space or a cmd.exe metacharacter
forces quotes, and an embedded double quote is replaced so quoting cannot
be broken out of. The first two cases pin the compatibility guarantee -
they are what a later "just quote everything" simplification would break.

Move wide_cmd_line() to win32-util.c next to wide_string(), which it
calls. The unit tests already link that file, so nothing else needs to
change to reach it.

Change-Id: Iba66235cbad52692da542abd47e9f51810b0ed66
Signed-off-by: Lev Stipakov <[email protected]>
---
M src/openvpn/win32-util.c
M src/openvpn/win32-util.h
M src/openvpn/win32.c
M tests/unit_tests/openvpn/test_argv.c
4 files changed, 128 insertions(+), 67 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/92/1892/1

diff --git a/src/openvpn/win32-util.c b/src/openvpn/win32-util.c
index 209e34e..4e3819c 100644
--- a/src/openvpn/win32-util.c
+++ b/src/openvpn/win32-util.c
@@ -45,6 +45,73 @@
     return ucs16;
 }
 
+/* special to cmd.exe, which CreateProcess() uses to run .bat/.cmd (VU#123335) */
+#define CMD_QUOTE_TRIGGERS " &|<>^%()!"
+
+static bool
+argv_element_needs_quotes(const char *str)
+{
+    for (const char *c = str; *c != '\0'; ++c)
+    {
+        if (strchr(CMD_QUOTE_TRIGGERS, *c) != NULL)
+        {
+            return true;
+        }
+    }
+    return false;
+}
+
+WCHAR *
+wide_cmd_line(const struct argv *a, struct gc_arena *gc)
+{
+    size_t nchars = 1;
+    size_t maxlen = 0;
+    size_t i;
+    struct buffer buf;
+    char *work = NULL;
+
+    if (!a)
+    {
+        return NULL;
+    }
+
+    for (i = 0; i < a->argc; ++i)
+    {
+        const char *arg = a->argv[i];
+        const size_t len = strlen(arg);
+        nchars += len + 3;
+        if (len > maxlen)
+        {
+            maxlen = len;
+        }
+    }
+
+    work = gc_malloc(maxlen + 1, false, gc);
+    check_malloc_return(work);
+    buf = alloc_buf_gc(nchars, gc);
+
+    for (i = 0; i < a->argc; ++i)
+    {
+        const char *arg = a->argv[i];
+        strcpy(work, arg);
+        string_mod(work, CC_PRINT, CC_DOUBLE_QUOTE | CC_CRLF, '_');
+        if (i)
+        {
+            buf_printf(&buf, " ");
+        }
+        if (argv_element_needs_quotes(work))
+        {
+            buf_printf(&buf, "\"%s\"", work);
+        }
+        else
+        {
+            buf_printf(&buf, "%s", work);
+        }
+    }
+
+    return wide_string(BSTR(&buf), gc);
+}
+
 char *
 utf16to8(const wchar_t *utf16, struct gc_arena *gc)
 {
diff --git a/src/openvpn/win32-util.h b/src/openvpn/win32-util.h
index fb83762..c2ad64b 100644
--- a/src/openvpn/win32-util.h
+++ b/src/openvpn/win32-util.h
@@ -24,11 +24,15 @@
 #ifndef OPENVPN_WIN32_UTIL_H
 #define OPENVPN_WIN32_UTIL_H
 
+#include "argv.h"
 #include "buffer.h"
 
 /* Convert a string from UTF-8 to UCS-2 */
 WCHAR *wide_string(const char *utf8, struct gc_arena *gc);
 
+/* Build a CreateProcess() command line from argv */
+WCHAR *wide_cmd_line(const struct argv *a, struct gc_arena *gc);
+
 /* Convert a string from UTF-16 to UTF-8 */
 char *utf16to8(const wchar_t *utf16, struct gc_arena *gc);
 
diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c
index d527160..80ec7e8 100644
--- a/src/openvpn/win32.c
+++ b/src/openvpn/win32.c
@@ -936,73 +936,6 @@
     }
 }
 
-/* special to cmd.exe, which CreateProcess() uses to run .bat/.cmd (VU#123335) */
-#define CMD_QUOTE_TRIGGERS " &|<>^%()!"
-
-static bool
-argv_element_needs_quotes(const char *str)
-{
-    for (const char *c = str; *c != '\0'; ++c)
-    {
-        if (strchr(CMD_QUOTE_TRIGGERS, *c) != NULL)
-        {
-            return true;
-        }
-    }
-    return false;
-}
-
-static WCHAR *
-wide_cmd_line(const struct argv *a, struct gc_arena *gc)
-{
-    size_t nchars = 1;
-    size_t maxlen = 0;
-    size_t i;
-    struct buffer buf;
-    char *work = NULL;
-
-    if (!a)
-    {
-        return NULL;
-    }
-
-    for (i = 0; i < a->argc; ++i)
-    {
-        const char *arg = a->argv[i];
-        const size_t len = strlen(arg);
-        nchars += len + 3;
-        if (len > maxlen)
-        {
-            maxlen = len;
-        }
-    }
-
-    work = gc_malloc(maxlen + 1, false, gc);
-    check_malloc_return(work);
-    buf = alloc_buf_gc(nchars, gc);
-
-    for (i = 0; i < a->argc; ++i)
-    {
-        const char *arg = a->argv[i];
-        strcpy(work, arg);
-        string_mod(work, CC_PRINT, CC_DOUBLE_QUOTE | CC_CRLF, '_');
-        if (i)
-        {
-            buf_printf(&buf, " ");
-        }
-        if (argv_element_needs_quotes(work))
-        {
-            buf_printf(&buf, "\"%s\"", work);
-        }
-        else
-        {
-            buf_printf(&buf, "%s", work);
-        }
-    }
-
-    return wide_string(BSTR(&buf), gc);
-}
-
 /*
  * Attempt to simulate fork/execve on Windows
  */
diff --git a/tests/unit_tests/openvpn/test_argv.c b/tests/unit_tests/openvpn/test_argv.c
index b1e3261..5b6e26e 100644
--- a/tests/unit_tests/openvpn/test_argv.c
+++ b/tests/unit_tests/openvpn/test_argv.c
@@ -14,6 +14,10 @@
 #include "buffer.h"
 #include "test_common.h"
 
+#ifdef _WIN32
+#include "win32-util.h"
+#endif
+
 /* Defines for use in the tests and the mock parse_line() */
 #define PATH1      "/s p a c e"
 #define PATH2      "/foo bar/baz"
@@ -248,6 +252,56 @@
     argv_free(&a);
 }
 
+#ifdef _WIN32
+/*
+ * An argument is quoted if and only if it holds a space or a character that
+ * cmd.exe would act on when CreateProcess() runs a .bat/.cmd target.
+ */
+static void
+wide_cmd_line__quotes_only_what_cmd_would_reinterpret(void **state)
+{
+    static const struct
+    {
+        const char *arg;
+        const WCHAR *expected;
+    } cases[] = {
+        /* nothing special - must stay unquoted, or existing scripts break */
+        { "CN=user1", L"script.bat 0 CN=user1" },
+        /* the batch delimiters are deliberately not triggers */
+        { "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 */
+        { "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" },
+    };
+
+    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
 main(void)
 {
@@ -267,6 +321,9 @@
         cmocka_unit_test(argv_str__multiple_argv__correct_output),
         cmocka_unit_test(argv_insert_head__non_empty_argv__head_added),
         cmocka_unit_test(argv_insert_head__empty_argv__head_only),
+#ifdef _WIN32
+        cmocka_unit_test(wide_cmd_line__quotes_only_what_cmd_would_reinterpret),
+#endif
     };
 
     return cmocka_run_group_tests_name("argv", tests, NULL, NULL);

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1892?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: Iba66235cbad52692da542abd47e9f51810b0ed66
Gerrit-Change-Number: 1892
Gerrit-PatchSet: 1
Gerrit-Owner: cron2 <[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