Attention is currently required from: plaisthos, stipa.
Hello plaisthos,
I'd like you to reexamine a change. Please visit
http://gerrit.openvpn.net/c/openvpn/+/1736?usp=email
to look at the new patch set (#2).
The following approvals got outdated and were removed:
Code-Review-1 by plaisthos
Change subject: win32: fix plugin trusted-dir check prefix bypass
......................................................................
win32: fix plugin trusted-dir check prefix bypass
plugin_in_trusted_dir() validated the plugin path against the trusted
plugin/install directory (and the system directory fallback) using a raw
string prefix match via wcsnicmp(). When the trusted directory path does
not end in a separator (e.g. the plugin_dir registry value is set to
"C:\openvpn_plugins"), a sibling directory sharing the same prefix
("C:\openvpn_plugins_evil") also passes the check, allowing a plugin to
be loaded from outside the allow-listed directory.
Introduce win_path_in_dir() in win32-util.c which performs the prefix
match but additionally requires the match to end on a path-component
boundary, and use it for both the plugin/install directory and the
system directory checks. Add unit tests in test_misc.c.
Change-Id: Ib7f9c9ce5ed778190445cc4cfaa8f3cd5d1110bc
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_misc.c
4 files changed, 85 insertions(+), 9 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/36/1736/2
diff --git a/src/openvpn/win32-util.c b/src/openvpn/win32-util.c
index 6fc3be4..209e34e 100644
--- a/src/openvpn/win32-util.c
+++ b/src/openvpn/win32-util.c
@@ -175,4 +175,30 @@
return tmpdir;
}
+bool
+win_path_in_dir(const WCHAR *path, const WCHAR *dir)
+{
+ size_t dir_len = wcslen(dir);
+ /* dir_len <= 1 guards the dir[dir_len - 1] access below and rejects a
+ * degenerate single-character directory (a normalized absolute path is
+ * always longer). */
+ if (dir_len <= 1 || wcsnicmp(dir, path, dir_len) != 0)
+ {
+ return false;
+ }
+
+ /* A plain prefix match is not sufficient: if dir is "C:\foo" then
+ * "C:\foo_evil\bar.dll" shares the prefix but is not inside "C:\foo".
+ * Require that the matched prefix ends on a path separator, i.e. either
+ * dir already ends with a separator or the character following the prefix
+ * in path is one. */
+ if (dir[dir_len - 1] == L'\\' || dir[dir_len - 1] == L'/')
+ {
+ return true;
+ }
+
+ WCHAR next = path[dir_len];
+ return next == L'\\' || next == L'/';
+}
+
#endif /* _WIN32 */
diff --git a/src/openvpn/win32-util.h b/src/openvpn/win32-util.h
index 6636cf3..4849ea9 100644
--- a/src/openvpn/win32-util.h
+++ b/src/openvpn/win32-util.h
@@ -42,5 +42,17 @@
/* Find temporary directory */
const char *win_get_tempdir(void);
+/**
+ * Check whether @p path resides within directory @p dir.
+ *
+ * Unlike a plain string prefix match, this requires the match to end on a
+ * path-component boundary, so that "C:\\foo_evil\\bar" is not considered to be
+ * inside "C:\\foo". Both arguments are expected to be normalized absolute
+ * paths. The comparison is case-insensitive.
+ *
+ * @return true if @p path is inside @p dir, false otherwise.
+ */
+bool win_path_in_dir(const WCHAR *path, const WCHAR *dir);
+
#endif /* OPENVPN_WIN32_UTIL_H */
#endif /* ifdef _WIN32 */
diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c
index b2a7331..ca08ead 100644
--- a/src/openvpn/win32.c
+++ b/src/openvpn/win32.c
@@ -1587,14 +1587,13 @@
}
/* Check if the plugin path resides within the plugin/install directory */
- if ((wcslen(normalized_plugin_dir) > 0)
- && (wcsnicmp(normalized_plugin_dir, plugin_path,
wcslen(normalized_plugin_dir)) == 0))
+ if (win_path_in_dir(plugin_path, normalized_plugin_dir))
{
return true;
}
/* Fallback to the system directory */
- return wcsnicmp(system_dir, plugin_path, wcslen(system_dir)) == 0;
+ return win_path_in_dir(plugin_path, system_dir);
}
bool
diff --git a/tests/unit_tests/openvpn/test_misc.c
b/tests/unit_tests/openvpn/test_misc.c
index ccdfdee..fc9840a 100644
--- a/tests/unit_tests/openvpn/test_misc.c
+++ b/tests/unit_tests/openvpn/test_misc.c
@@ -38,6 +38,9 @@
#include "test_common.h"
#include "list.h"
#include "mock_msg.h"
+#ifdef _WIN32
+#include "win32-util.h"
+#endif
static void
test_compat_lzo_string(void **state)
@@ -445,12 +448,48 @@
mock_set_debug_level(saved_log_level);
}
-const struct CMUnitTest misc_tests[] = {
cmocka_unit_test(test_compat_lzo_string),
-
cmocka_unit_test(test_auth_fail_temp_no_flags),
-
cmocka_unit_test(test_auth_fail_temp_flags),
-
cmocka_unit_test(test_auth_fail_temp_flags_msg),
- cmocka_unit_test(test_list),
- cmocka_unit_test(test_atoi_variants)
};
+#ifdef _WIN32
+static void
+test_win_path_in_dir(void **state)
+{
+ /* plugin/install dir without trailing separator */
+ assert_true(win_path_in_dir(L"C:\\openvpn_plugins\\foo.dll",
L"C:\\openvpn_plugins"));
+
+ /* the bug being fixed: a sibling dir sharing the prefix must NOT match */
+ assert_false(win_path_in_dir(L"C:\\openvpn_plugins_evil\\foo.dll",
L"C:\\openvpn_plugins"));
+
+ /* trusted dir with trailing separator */
+ assert_true(win_path_in_dir(L"C:\\openvpn_plugins\\foo.dll",
L"C:\\openvpn_plugins\\"));
+ assert_false(win_path_in_dir(L"C:\\openvpn_plugins_evil\\foo.dll",
L"C:\\openvpn_plugins\\"));
+
+ /* forward slash separator in the candidate path is accepted */
+ assert_true(win_path_in_dir(L"C:\\openvpn_plugins/foo.dll",
L"C:\\openvpn_plugins"));
+
+ /* comparison is case-insensitive */
+ assert_true(win_path_in_dir(L"c:\\OPENVPN_PLUGINS\\foo.dll",
L"C:\\openvpn_plugins"));
+
+ /* the directory itself (no trailing component) is not "in" the directory
*/
+ assert_false(win_path_in_dir(L"C:\\openvpn_plugins",
L"C:\\openvpn_plugins"));
+
+ /* nested subdirectories are still inside */
+ assert_true(win_path_in_dir(L"C:\\openvpn_plugins\\sub\\foo.dll",
L"C:\\openvpn_plugins"));
+
+ /* an empty trusted dir never matches */
+ assert_false(win_path_in_dir(L"C:\\openvpn_plugins\\foo.dll", L""));
+}
+#endif /* _WIN32 */
+
+const struct CMUnitTest misc_tests[] = {
+#ifdef _WIN32
+ cmocka_unit_test(test_win_path_in_dir),
+#endif
+ cmocka_unit_test(test_compat_lzo_string),
+ cmocka_unit_test(test_auth_fail_temp_no_flags),
+ cmocka_unit_test(test_auth_fail_temp_flags),
+ cmocka_unit_test(test_auth_fail_temp_flags_msg),
+ cmocka_unit_test(test_list),
+ cmocka_unit_test(test_atoi_variants)
+};
int
main(void)
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1736?usp=email
To unsubscribe, or for help writing mail filters, visit
http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ib7f9c9ce5ed778190445cc4cfaa8f3cd5d1110bc
Gerrit-Change-Number: 1736
Gerrit-PatchSet: 2
Gerrit-Owner: stipa <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: cron2 <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
Gerrit-Attention: stipa <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
