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 (#5).
The following approvals got outdated and were removed:
Code-Review+2 by Antonio Quartulli
The change is no longer submittable: Code-Review and checks~ChecksSubmitRule
are unsatisfied now.
Change subject: openvpnserv: reject a config file passed without --config
......................................................................
openvpnserv: reject a config file passed without --config
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 <[email protected]>
---
M src/openvpnserv/interactive.c
M tests/unit_tests/openvpnserv/test_openvpnserv.c
2 files changed, 54 insertions(+), 14 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/59/2159/5
diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 81205d6..f753d46 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -355,6 +355,23 @@
}
/*
+ * openvpn.exe takes a lone argument as the config file only when there are
+ * no other arguments, and takes non-options after --msg-channel <handle> as
+ * its extra parameters.
+ */
+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
* The options are taken from the command line openvpn is started with,
@@ -396,20 +413,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 +3842,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);
--
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: 5
Gerrit-Owner: Răzvan Cojocaru <[email protected]>
Gerrit-Reviewer: Antonio Quartulli <[email protected]>
Gerrit-Reviewer: Arne Schwabe <[email protected]>
Gerrit-CC: Heiko Hund <[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
