diff mbox series

[Openvpn-devel,v7] openvpnserv: reject a config file passed without --config

Message ID 20261112214027.13465-1-gert@greenie.muc.de
State New
Headers
Series [Openvpn-devel,v7] openvpnserv: reject a config file passed without --config |

Commit Message

Gert Doering Nov. 12, 2026, 9:40 p.m. UTC 
  From: Razvan Cojocaru <razvanc@mailbox.org>

openvpn.exe takes a lone argument as the config file only when there
are no other arguments, but the service adds --msg-channel. Since
c139e82d openvpn.exe takes the config file as an extra parameter of
--msg-channel and ignores it. ValidateOptions() treated it as the
config file, but only ever returned FALSE for it, with an empty error
message if the path was fine.

Return an error to all users when the options don't start with an
option, and drop the lone argument special case in ValidateOptions(),
so that a lone option is checked against the white list.

Change-Id: I57469ca50c551ad475510690ecfa2d5e7cf9cfcf
Signed-off-by: Razvan Cojocaru <razvanc@mailbox.org>
Acked-by: Heiko Hund <heiko@ist.eigentlich.net>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/2159
---

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

Acked-by according to Gerrit (reflected above):
Heiko Hund <heiko@ist.eigentlich.net>
diff mbox series

Patch

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 81205d6..9de747e 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -354,6 +354,29 @@ 
     ReturnError(pipe, GetLastError(), func, 1, &exit_event);
 }
 
+/**
+ * Check whether the user options start with a non-option
+ *
+ * openvpn.exe takes a lone argument as the config file only when there are
+ * no other arguments, and takes non-options after the --msg-channel handle
+ * as its extra parameters.
+ *
+ * @param  cmdline  openvpn.exe command line built by the service
+ *
+ * @return BOOL to indicate if the first user argument is not an option
+ */
+static BOOL
+StartsWithNonOption(const WCHAR *cmdline)
+{
+    int argc;
+    WCHAR **argv = CommandLineToArgvW(cmdline, &argc);
+    /* "openvpn --msg-channel <handle> <arg>..." */
+    BOOL ret = argv && argc > 3 && !IsOption(argv[3]);
+
+    LocalFree(argv);
+    return ret;
+}
+
 /*
  * Validate options against a white list. Also check the config_file is
  * inside the config_dir. The white list is defined in validate.c
@@ -396,20 +419,6 @@ 
         goto out;
     }
 
-    /*
-     * If only one argument, it is the config file
-     */
-    if (argc == first + 1)
-    {
-        WCHAR *argv_tmp[2] = { L"--config", argv[first] };
-
-        if (!CheckOption(workdir, 2, argv_tmp, &settings))
-        {
-            swprintf(errmsg, capacity, msg1, argv[first], workdir, settings.ovpn_admin_group);
-        }
-        goto out;
-    }
-
     for (i = first; i < argc; ++i)
     {
         if (!IsOption(argv[i]))
@@ -3839,6 +3848,14 @@ 
     swprintf(cmdline, cmdline_size, L"openvpn --msg-channel %" PRIuPTR L" %ls",
              (uintptr_t)svc_pipe, sud.options);
 
+    if (StartsWithNonOption(cmdline))
+    {
+        ReturnError(pipe, ERROR_STARTUP_DATA,
+                    L"Options must start with an option, pass the config file with --config", 1,
+                    &exit_event);
+        goto out;
+    }
+
     /*
      * Only authorized users are allowed to use any command line options or
      * have the config file in locations other than the global config directory.
diff --git a/tests/unit_tests/openvpnserv/test_openvpnserv.c b/tests/unit_tests/openvpnserv/test_openvpnserv.c
index 20fb1af..7db61f7 100644
--- a/tests/unit_tests/openvpnserv/test_openvpnserv.c
+++ b/tests/unit_tests/openvpnserv/test_openvpnserv.c
@@ -301,6 +301,33 @@ 
     assert_merge_result(L"", L"", L"");
 }
 
+static void
+test_starts_with_non_option(void **state)
+{
+    assert_true(StartsWithNonOption(L"openvpn --msg-channel 1 foo.ovpn"));
+    assert_true(StartsWithNonOption(L"openvpn --msg-channel 1 \"my vpn.ovpn\""));
+    assert_true(StartsWithNonOption(L"openvpn --msg-channel 1 foo.ovpn --verb 3"));
+    assert_true(StartsWithNonOption(L"openvpn --msg-channel 1 foo.ovpn bar"));
+    assert_true(StartsWithNonOption(L"openvpn --msg-channel 1 \"\""));
+
+    assert_false(StartsWithNonOption(L"openvpn --msg-channel 1 "));
+    assert_false(StartsWithNonOption(L"openvpn --msg-channel 1 --persist-tun"));
+    assert_false(StartsWithNonOption(L"openvpn --msg-channel 1 --config foo.ovpn"));
+}
+
+static void
+test_validate_lone_option(void **state)
+{
+    WCHAR errmsg[512];
+
+    assert_true(ValidateOptions(NULL, L"C:\\OpenVPN\\config\\",
+                                L"openvpn --msg-channel 1 --script-security", errmsg,
+                                _countof(errmsg)));
+    assert_false(ValidateOptions(NULL, L"C:\\OpenVPN\\config\\",
+                                 L"openvpn --msg-channel 1 --persist-tun", errmsg,
+                                 _countof(errmsg)));
+}
+
 int
 wmain(void)
 {
@@ -311,6 +338,8 @@ 
         cmocka_unit_test(test_append_search_list),
         cmocka_unit_test(test_remove_search_list_tokens),
         cmocka_unit_test(test_merge_openvpn_environment),
+        cmocka_unit_test(test_starts_with_non_option),
+        cmocka_unit_test(test_validate_lone_option),
     };
 
     int ret = cmocka_run_group_tests_name("openvpnserv tests", tests, NULL, NULL);