Attention is currently required from: Antonio Quartulli, Arne Schwabe, Răzvan
Cojocaru.
Hello Antonio Quartulli, Arne Schwabe,
I'd like you to reexamine a change. Please visit
https://gerrit.openvpn.net/c/openvpn/+/2159?usp=email
to look at the new patch set (#2).
The following approvals got outdated and were removed:
Code-Review-1 by Antonio Quartulli
Change subject: openvpnserv: pass a lone config file to openvpn with --config
......................................................................
openvpnserv: pass a lone config file to openvpn with --config
openvpn.exe takes a lone argument as the config file only when there
are no other arguments, but the service adds --msg-channel. Before
c139e82d openvpn.exe rejected the config file for lacking a leading
"--", since then it takes it as an extra parameter of --msg-channel and
ignores it. ValidateOptions() treated it as the config file, but only
ever returned FALSE for it.
Put --config in front of a lone non-option when building the command
line, so that ValidateOptions() checks what openvpn.exe gets, and drop
its special case for a lone argument.
Change-Id: I57469ca50c551ad475510690ecfa2d5e7cf9cfcf
Signed-off-by: Razvan Cojocaru <[email protected]>
---
M src/openvpnserv/interactive.c
M tests/unit_tests/openvpnserv/test_openvpnserv.c
2 files changed, 77 insertions(+), 22 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/59/2159/2
diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 81205d6..f9e783a 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -355,6 +355,43 @@
}
/*
+ * Build the command line openvpn is started with. openvpn.exe takes a lone
+ * argument as the config file only when there are no other arguments, so a
+ * lone non-option gets a --config in front. It is put in front of the
+ * unsplit options, so that they are split the same way with and without it.
+ * Returns the command line to be freed by the caller, or NULL on error.
+ */
+static WCHAR *
+BuildCmdline(HANDLE svc_pipe, const WCHAR *options)
+{
+ /* there seem to be no common printf specifier that works on all
+ * mingw/msvc platforms without trickery, so convert to void* and use
+ * PRIuPTR to print that as best compromise */
+ const WCHAR *fmt = L"openvpn --msg-channel %" PRIuPTR L" %ls%ls";
+ size_t size = wcslen(options) + 128;
+ WCHAR *cmdline = malloc(size * sizeof(*cmdline));
+ WCHAR **argv;
+ int argc;
+
+ if (!cmdline)
+ {
+ return NULL;
+ }
+
+ swprintf(cmdline, size, fmt, (uintptr_t)svc_pipe, L"", options);
+
+ /* "openvpn --msg-channel <handle> <arg>" */
+ argv = CommandLineToArgvW(cmdline, &argc);
+ if (argv && argc == 4 && !IsOption(argv[3]))
+ {
+ swprintf(cmdline, size, fmt, (uintptr_t)svc_pipe, L"--config ",
options);
+ }
+ LocalFree(argv);
+
+ return cmdline;
+}
+
+/*
* Validate options against a white list. Also check the config_file is
* inside the config_dir. The white list is defined in validate.c
* The options are taken from the command line openvpn is started with,
@@ -396,20 +433,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]))
@@ -3567,7 +3590,6 @@
according to MSDN. */
LPCWSTR exe_path;
WCHAR *cmdline = NULL;
- size_t cmdline_size;
undo_lists_t undo_lists;
WCHAR errmsg[512] = L"";
BOOL flush_pipe = TRUE;
@@ -3826,18 +3848,12 @@
goto out;
}
- cmdline_size = wcslen(sud.options) + 128;
- cmdline = malloc(cmdline_size * sizeof(*cmdline));
+ cmdline = BuildCmdline(svc_pipe, sud.options);
if (cmdline == NULL)
{
ReturnLastError(pipe, L"malloc");
goto out;
}
- /* there seem to be no common printf specifier that works on all
- * mingw/msvc platforms without trickery, so convert to void* and use
- * PRIuPTR to print that as best compromise */
- swprintf(cmdline, cmdline_size, L"openvpn --msg-channel %" PRIuPTR L" %ls",
- (uintptr_t)svc_pipe, sud.options);
/*
* Only authorized users are allowed to use any command line options or
diff --git a/tests/unit_tests/openvpnserv/test_openvpnserv.c
b/tests/unit_tests/openvpnserv/test_openvpnserv.c
index 20fb1af..82c398e 100644
--- a/tests/unit_tests/openvpnserv/test_openvpnserv.c
+++ b/tests/unit_tests/openvpnserv/test_openvpnserv.c
@@ -301,6 +301,44 @@
assert_merge_result(L"", L"", L"");
}
+static void
+assert_cmdline(const WCHAR *options, const WCHAR *expected, BOOL valid)
+{
+ WCHAR errmsg[512];
+ WCHAR *cmdline = BuildCmdline((HANDLE)1, options);
+
+ assert_non_null(cmdline);
+ assert_int_equal(wcscmp(cmdline, expected), 0);
+ assert_int_equal(ValidateOptions(NULL, settings.config_dir, cmdline,
errmsg, _countof(errmsg)),
+ valid);
+ free(cmdline);
+}
+
+static void
+test_build_validate_cmdline(void **state)
+{
+ wcscpy(settings.config_dir, L"C:\\OpenVPN\\config\\");
+ wcscpy(settings.ovpn_admin_group, L"OpenVPN Administrators");
+
+ /* a lone non-option is the config file */
+ assert_cmdline(L"foo.ovpn", L"openvpn --msg-channel 1 --config foo.ovpn",
TRUE);
+ assert_cmdline(L"\"my vpn.ovpn\"", L"openvpn --msg-channel 1 --config \"my
vpn.ovpn\"", TRUE);
+ assert_cmdline(L"..\\evil.ovpn", L"openvpn --msg-channel 1 --config
..\\evil.ovpn", FALSE);
+ assert_cmdline(L"stdin", L"openvpn --msg-channel 1 --config stdin", FALSE);
+
+ /* a lone option is checked against the white list */
+ assert_cmdline(L"--script-security", L"openvpn --msg-channel 1
--script-security", TRUE);
+ assert_cmdline(L"--persist-tun", L"openvpn --msg-channel 1 --persist-tun",
FALSE);
+ assert_cmdline(L"\"--persist-tun\"", L"openvpn --msg-channel 1
\"--persist-tun\"", FALSE);
+
+ /* other options are passed unchanged */
+ assert_cmdline(L"", L"openvpn --msg-channel 1 ", TRUE);
+ assert_cmdline(L"--config foo.ovpn --verb 3",
+ L"openvpn --msg-channel 1 --config foo.ovpn --verb 3",
TRUE);
+
+ memset(&settings, 0, sizeof(settings));
+}
+
int
wmain(void)
{
@@ -311,6 +349,7 @@
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_build_validate_cmdline),
};
int ret = cmocka_run_group_tests_name("openvpnserv tests", tests, NULL,
NULL);
--
To view, visit https://gerrit.openvpn.net/c/openvpn/+/2159?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I57469ca50c551ad475510690ecfa2d5e7cf9cfcf
Gerrit-Change-Number: 2159
Gerrit-PatchSet: 2
Gerrit-Owner: Răzvan Cojocaru <[email protected]>
Gerrit-Reviewer: Antonio Quartulli <[email protected]>
Gerrit-Reviewer: Arne Schwabe <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: Antonio Quartulli <[email protected]>
Gerrit-Attention: Răzvan Cojocaru <[email protected]>
Gerrit-Attention: Arne Schwabe <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
